Conversation
timbms
force-pushed
the
migrateToAppIntents2
branch
from
2a7609c to
8d77998
Compare
There was a problem hiding this comment.
Pull request overview
This PR migrates the openHAB iOS application from the legacy Intents framework to the modern App Intents framework introduced in iOS 16. The migration removes the separate Intents extension target and replaces it with native App Intents implementations directly in the main application.
Key changes:
- Removes the openHABIntents extension target including all handler classes, configuration files, and tests
- Adds eight new AppIntent implementations in the AppIntents folder for controlling openHAB items
- Updates the Xcode project to remove the extension target and integrate the new App Intents
Reviewed changes
Copilot reviewed 21 out of 22 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| openHABWatch/Domain/UserData.swift | Minor whitespace cleanup in a comment |
| openHABIntentsTests/SetSwitchStateIntentHandlerTests.swift | Removed test file for legacy intent handler |
| openHABIntents/openHABIntents.entitlements | Removed entitlements for legacy intents extension |
| openHABIntents/SetSwitchStateIntentHandler.swift | Removed legacy switch state intent handler |
| openHABIntents/SetStringValueIntentHandler.swift | Removed legacy string value intent handler |
| openHABIntents/SetNumberValueIntentHandler.swift | Removed legacy number value intent handler |
| openHABIntents/SetDimmerRollerValueIntentHandler.swift | Removed legacy dimmer/roller intent handler |
| openHABIntents/SetContactStateValueIntentHandler.swift | Removed legacy contact state intent handler |
| openHABIntents/SetColorValueIntentHandler.swift | Removed legacy color value intent handler |
| openHABIntents/OpenHABIntentHelper.swift | Removed legacy intent helper utilities |
| openHABIntents/IntentHandler.swift | Removed main intent extension handler |
| openHABIntents/Info.plist | Removed extension configuration |
| openHABIntents/GetItemStateIntentHandler.swift | Removed legacy get item state intent handler |
| openHAB.xcodeproj/project.pbxproj | Updated project to remove intents extension target and add new App Intents files |
| AppIntents/SetSwitchState.swift | New App Intent for setting switch states (on/off) |
| AppIntents/SetStringValue.swift | New App Intent for setting string item values |
| AppIntents/SetNumberValue.swift | New App Intent for setting number item values |
| AppIntents/SetDimmerRollerValue.swift | New App Intent for setting dimmer/roller shutter values |
| AppIntents/SetContactStateValue.swift | New App Intent for setting contact states |
| AppIntents/SetColorValue.swift | New App Intent for setting color values in HSB format |
| AppIntents/Home.swift | New App Entity representing an openHAB home configuration |
| AppIntents/GetItemState.swift | New App Intent for retrieving item states |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 23 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 28 changed files in this pull request and generated 8 comments.
Files not reviewed (1)
- openHAB.xcworkspace/contents.xcworkspacedata: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 40 out of 42 changed files in this pull request and generated 10 comments.
Files not reviewed (1)
- openHAB.xcworkspace/contents.xcworkspacedata: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 37 out of 39 changed files in this pull request and generated 3 comments.
Files not reviewed (1)
- openHAB.xcworkspace/contents.xcworkspacedata: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Move Firebase, push notifications, audio session, watch connectivity, and screensaver setup into a deferred Task so the UI renders faster. Add a redacted placeholder title in SitemapPageView while loading, and use EmbeddingRowView for placeholder rows. Signed-off-by: Tim Mueller-Seydlitz <timbms@gmail.com>
… title - Remove untracked `for await` Task from SitemapPageViewModel.init() that leaked and duplicated connection handling already done by .onChange - Always show TextField in TextInputRowView instead of gating on labelValue - Conditionally apply .searchable in SitemapNavigationView based on the showSearchField preference - Store linked-page title as defaultSitemapLabel so pageTitle returns it immediately Signed-off-by: Tim Mueller-Seydlitz <timbms@gmail.com>
Restore for-await connection observer in SitemapPageViewModel with proper Task tracking (cancelled in deinit) to fix startup hang. Root page observes all emissions for initial load; linked pages use dropFirst() to defer loading until the view appears via .task. Remove unreliable .onChange observer from SitemapPageView. Return empty pageTitle when no label is available so the navigation bar shows a redacted placeholder instead of the raw sitemap name. Signed-off-by: Tim Mueller-Seydlitz <timbms@gmail.com>
Use dropFirst() so for-await only handles connection changes, not initial load. Remove connectionObserverTask from linked-page init to avoid eager loading. Restore unconditional .task for all pages. Add URL guard in handleActiveConnectionChange to skip when NetworkTracker re-evaluates to the same connection, preventing long-polling restarts. Set openHABRootUrl in startPageHandling so the guard works for .task initiated loads. Signed-off-by: Tim Mueller-Seydlitz <timbms@gmail.com>
Eliminates the empty black screen between SwiftUI view creation and .task firing by starting in the loading state. The skeleton condition (isLoading && relevantWidgets.isEmpty) still correctly shows content for the preview init which populates widgets directly. Signed-off-by: Tim Mueller-Seydlitz <timbms@gmail.com>
…data Delete openHAB/PreviewConstants.swift which duplicated CommonUI's #if DEBUG-guarded version and shipped preview JSON in Release builds. Replace placeholderWidgets with lightweight static widgets constructed inline, removing the PreviewConstants dependency from SitemapPageView. Remove the fake inline placeholder title that was offset from the navigation bar title. Signed-off-by: Tim Mueller-Seydlitz <timbms@gmail.com>
Replace tertiarySystemBackground/secondarySystemBackground with systemGray4 (dark) and systemGray5 (light) for slightly improved track visibility in both color schemes. Signed-off-by: Tim Mueller-Seydlitz <timbms@gmail.com>
Render inline buttons directly in SegmentRow for single-mapping and press-release scenarios instead of always navigating to the full-screen SegmentSelectionView. Multi-segment (2+ regular mappings) retains the existing NavigationLink flow. Layout adapts based on label presence: VStack with title row when labeled, HStack when not. Signed-off-by: Tim Mueller-Seydlitz <timbms@gmail.com>
Add contentShape(RoundedRectangle) to inlineButton so DragGesture and Button tap targets are clipped to the visible button bounds, preventing touches on the title row above from triggering button actions. Signed-off-by: Tim Mueller-Seydlitz <timbms@gmail.com>
Move gesture recognizers into an overlay on the button view so the hit-test area is physically bounded to the button frame. This prevents touches on the title row above from triggering button actions on watchOS. Signed-off-by: Tim Mueller-Seydlitz <timbms@gmail.com>
Use GeometryReader inside gesture overlays to check that the touch startLocation falls within the button bounds, rejecting touches routed from outside (e.g. the title row above) by watchOS list row hit testing. Signed-off-by: Tim Mueller-Seydlitz <timbms@gmail.com>
Signed-off-by: Tim Mueller-Seydlitz <timbms@gmail.com>
In your first screenshot it says]
where it should already be
as Home is the name of your only/currently active home. There should not be any need to tap on the blue Zuhause to select the correct Home in the popup. I. e. actual home names should never be localised. BTW @TAKeanice Are you also generally running the openHAB app in English, with your home name being Home? |
Signed-off-by: Tim Mueller-Seydlitz <timbms@gmail.com>
Signed-off-by: Tim Mueller-Seydlitz <timbms@gmail.com>
|
After the last commits I am still seeing a localised home name in German, but more importantly: when tapping on the Items variable in a shortcut it brings up an empty sheet, no Items listed whatsoever, in German or English. Can anyone reproduce this? |
|
And looking further into the localised home name: • The actual home name does not even seem to be taken into consideration as the default value for the selected game in shortcuts, but it always just seems to be the default unusual name given to any newly created home. I assume this because renaming the home name in the openHAb app to something other than "Home" does not affect the default name for newly created shortcuts. Only the subsequent popup does have the correct renamed home name. This should also be the name appearing as the already selected default home in Germany as well as in English. So this not might not be a localisation issue after all, but just a default selection not honouring actual home names at all. |
|
@DigiH I think you still got the part with the "Zuhause" wrong: It is not the name of a randomly chosen or default home, but the name of the prompt or placeholder when no home is selected. It's perfectly valid to have that localized, only the text might be clearer to avoid exactly the confusion you seem to have. But I also think it should not be necessary to select any value for the home variable. The way this worked previously in the old logic of INIntents was that unique items from any home are not prompting a query for the home when triggering the shortcut. Only if there were two identically named items, it would conflict and prompt for a home. I also wrote this in number 4 of my improvement recommendations. |
If that is really the case then I definitely have got it working ;) But why would it be that way? So anyone who keeps the default home name assigned it, which it initially gets, does get the benefit of not having to worry about assigning homes though the popup, while anybody who wants a more personalised home name has to go through the popup every time they want to create a shortcut? And as we already do have the correct home name in the popup, why not populate it directly as the home, especially if there is only one home. And aren't homes defined by a unique UUID in the background to their name? Let's say I have a home in a rural area, but then decide to get a second home in the middle of the city. Then I might want to rename my default rural home "Home" to "Country Home" - would that mean that all shortcut assigned homes would then have to be manually adjusted? This is not the case in the current SiriKit Intents after renaming a home, even though the home name in the shortcut stays the old name it works correctly with the new home name - which makes it even more confusing for me than before. So why would the name aways be the default name when, after choosing the correct name from the popup it does stay the proper chosen correct name, even after closing the shortcut and opening it again?!? I'm sorry, but your last comment made me even more confused now; as to what the Home row in the shortcuts should actually signify and how it should display and work 😕 |
Signed-off-by: Tim Mueller-Seydlitz <timbms@gmail.com>
Let me make another explanation attempt. If you see "Home" in the english UI, you see "Zuhause" in the German UI. Both mean no home has been selected. English users just have the downside that they cannot distinguish if their home called "Home" or no home is selected, because in both cases the UI shows "Home" and not even the color changes. In the German UI it would be distinguishable because as you saw in my screenshots, without a home selected the UI shows "Zuhause". Both behave the same way (in INIntents at least they did): |
is it really though?
But what about the home names of already existing shortcuts? Let me take you trough a quick excercies with TestFlight 3.2.20 - I assume the latest official release might behave the same - in English to avoid any possible localisation issue • I set up a new home in the app, it does get the default home name "Home" And I do know that when renaming a home it does state that this might break any existing shortcuts, but why would I even tap and hold on …, then select Edit and the expand the home row in the opened shortcut, when just tapping the shortcut confirms that it is running fine. Also how all these different home names will migrate to the new App Intents remains to be seen. And if this wasn't confusing enough already I am now planning to add another home of my friends or family to the app, which has some identical item names but not all. So all of a sudden now some of my "Home" shortcuts still work, while other "Home" shortcuts fail, same with some of the Crib and Pad shortcuts. O do know the reason why because you shared the insider info further above, but how this should also migrate to the new App Intents with all the different assigned home names is to be seen. So is there really a UUID in the background or is it just the insider info you shared above about the assigned home not not really being taken into account when there is only one home present? I also know that my exercise above is on the extreme side, but any user who has created at least one shortcut in the past and has renamed their home at least once could face this to some extend. If you now tell me that all this is working as intended and it is only me who finds this wildly confusing, and that all these possible different home names in the shortcuts won't be an issue for the App Intent migration, then I am fine with that and won't go on any further. I only do have two shortcuts for my hoe which get tied in with some automations, so worse comes to worst they can easily be recreated very quickly. |
Gosh, you could be right. I probably wanted to use UUIDs to reference homes in the INIntents version but encountered technical limitations. The warning and that unique items work without selected home mitigated this caveat. I will check that this weekend to confirm it. |
* Use stable cross-device identifier for Home AppEntity Shortcuts synced via iCloud between devices fail with "Invalid home" because Home.id was a device-local UUID that does not exist on the receiving device. Home.id now uses a stable identifier derived from the home's server configuration (cloudUserId > remote URL > local URL > UUID fallback) so that a shortcut created on one device resolves correctly on any other device pointing at the same server. Migration is automatic: HomeQuery.entities(for:) matches the stable identifier first, then falls back to a UUID string for shortcuts created before this fix. resolvedHomeId/resolveHomeId gain the same dual-lookup logic. The testable sync overload of resolvedHomeId accepts an explicit storedHomes parameter (defaulting to [:]) so unit tests continue to work via the UUID fallback without Preferences setup. Closes #1125 Signed-off-by: Tim Mueller-Seydlitz <timbms@gmail.com> * Fix @mainactor isolation errors in HomeResolver HomePreferences is @mainactor, so its properties (stableIdentifier, id) cannot be accessed from nonisolated contexts. Replace the storedHomes: [UUID: HomePreferences] parameter on the testable resolvedHomeId overload with a pre-computed stableIdentifierToLocalUUID: [(String, UUID)] array, which only holds plain Sendable values. The @mainactor production overload builds this map from Preferences.shared.storedHomes before delegating. Move the HomePreferences property accesses in the resolveHomeId production closure inside MainActor.run so they remain on the main actor. Signed-off-by: Tim Mueller-Seydlitz <timbms@gmail.com> * Use resolved homeId in sendCommand calls Replaces itemEntity.homeId with the homeId returned by resolvedHomeId in all intent perform() methods. The two values are equivalent after successful validation, but using the resolved value removes the "immutable value was never used" compiler warning and makes the data flow explicit. Signed-off-by: Tim Mueller-Seydlitz <timbms@gmail.com> * Fix localization test loading Localizable.xcstrings locally Adds LocalizableTestCatalog.json (symlink to Localizable.xcstrings) in test resources so the test bundle can load the catalog via Bundle API, avoiding iOS Simulator restrictions on host-filesystem access via #filePath. The #filePath path is kept as fallback. Also removes the unused "Invalid home identifier" localization entry (HomeResolutionError.invalidHomeIdentifier was renamed to unknownHome). Signed-off-by: Tim Mueller-Seydlitz <timbms@gmail.com> --------- Signed-off-by: Tim Mueller-Seydlitz <timbms@gmail.com>
|
@DigiH @TAKeanice Can you please test the latest commit and give feedback . |
|
I am currently not getting any Items at all when calling up the Items list for selection in a shortcut. This makes it impossible to test any further - unless this is a Simulator specific issue again. Tested in English and German with and existing home, but also with resetting the Simulator and creating a home from scratch. |
Two regressions prevented items from appearing in the Shortcuts item picker: 1. All ten ItemEntityQuery implementations derived selectedHomeId via UUID(uuidString: intent.home.id). After the stable cross-device identifier commit, Home.id is a URL or cloud user ID rather than a UUID string, so UUID(uuidString:) returned nil. The home-scoped query path was therefore never taken. Fix: replace the selectedHomeId: UUID? protocol requirement with selectedHome: Home?, and resolve the local UUID in a new @mainactor helper resolvedSelectedHomeId() that first matches by stableIdentifier against storedHomes, then falls back to UUID string parsing for shortcuts created before this change. 2. suggestedEntities() called getCachedOrPersistedItems() which only reads persisted stubs and never triggers a network fetch. On a fresh Simulator (or after reinstall) the stubs are empty, so the picker showed "No options available". Fix: call reloadCacheIfNeeded(homes:) before reading from the cache in both the home-scoped and all-homes paths of suggestedEntities(). The reload respects the 20 s TTL, so no extra network traffic occurs when the cache is already fresh. Signed-off-by: Tim Mueller-Seydlitz <timbms@gmail.com>
My mistake. Try again please |
|
I did get items when I just tested will comment my originally reported problems to keep a clean record
❌ Still the same.
🟢 Still the case but as argued in great length with @DigiH , that is correct behavior. The home name can be changed while the shortcut keeps working.
🟢 The new AppIntent code works equally fast if a home is selected! The timeout if some home is not reachable is 5s. It feels long but I think is necessary for situations with bad connection or a slow OH instance.
🟢 The search can find if I have an item named "Office Sensor" with the identifier "temperature_sensor_office" and I type "ic sens" or "temp". Great!
🟢 I figure the item includes the home UUID now. That´s a valid solution, too. No ambiguity possible. |
Signed-off-by: Tim Mueller-Seydlitz <timbms@gmail.com>
Signed-off-by: Tim Mueller-Seydlitz <timbms@gmail.com>
Thanks, Item list coming upon fine here as well now. But selecting a home in the popup in a shortcut clears the previously selected Item now, requiring to search and select the Item again. |
Signed-off-by: Tim Mueller-Seydlitz <timbms@gmail.com>
Signed-off-by: Tim Mueller-Seydlitz <timbms@gmail.com>
This PR migrates the openHAB iOS application from the legacy Siri Intents to the modern App Intents framework. The migration removes the separate Intents extension target and replaces it with native App Intents implementations directly in the main application.
Key changes:
BTW: This PR eliminates the last compiler warnings related to the Apple generated code from the legacy Siri Intents.
Depends on #892